Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added Population Component (Specific to MR) #77

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

agent-q1
Copy link
Contributor

@agent-q1 agent-q1 commented Jul 3, 2020

Population Component

Note: This has not been tested (due to technical reasons). Would be great if somebody else tested it.

@skaldarnar skaldarnar added the Type: Improvement Request for or addition/enhancement of a feature label Jul 4, 2020
Copy link
Contributor

@skaldarnar skaldarnar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good start 🙃 left a bunch of comments 😉

public class PopulationSystem extends BaseComponentSystem {

@In
LocalPlayer player;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when a authority system gets the LocalPlayer injected? I've never asked myself that question, now I'm curious 🤔 anybody knows?

I'm a bit concerned this may cause trouble in multi-player.

public void onPlayerSpawn(OnActivatedComponent event, EntityRef entity, ClientComponent component) {

entity.addComponent(new PopulationComponent());
logger.error("Component PopulationComponent added to player");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is obviously not an error 😅 (I guess this just to see that it actually works).

@ReceiveEvent
public void citizenSpawned(CitizenSpawnedEvent event, EntityRef citizen) {
FactionAlignmentComponent factionAlignmentComponent = citizen.getComponent(FactionAlignmentComponent.class);
PopulationComponent populationComponent = player.getClientEntity().getComponent(PopulationComponent.class);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know whether there is already a settlement entity in MetalRenegades, but my gut feeling says it would be nice to get the affected settlement from the CitizenSpawnedEvent. This settlement entity (or component) would then have a reference to the population:

        PopulationComponent populationComponent = event.getSettlement().getPopulation();

(please don't take the exact syntax for granted)

Copy link
Collaborator

@mayant15 mayant15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Remember to subtract from this component on citizen death too, once #76 is merged.

*
* The population is usually tracked by the {@link PopulationSystem}.
*/
public class MetalRenegadesPopulationComponent implements Component {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like FactionDistributionComponent might be a better name for this?


EntityRef homeEntity = homeComponent.building;
SettlementRefComponent settlementRefComponent = homeEntity.getComponent(SettlementRefComponent.class);
EntityRef settlementEntity = settlementRefComponent.settlement;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be cleaner to pass the settlement entity and the faction alignment through the CitizenSpawnedEvent. This event is only emitted here and all that information is available.

@agent-q1 agent-q1 requested review from mayant15 and skaldarnar July 16, 2020 15:31
Copy link
Collaborator

@mayant15 mayant15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering this is a relatively small change, it'll be better if these commits were squashed, especially when a lot of them are just "update" commits.

import org.terasology.entitySystem.event.Event;
import org.terasology.protobuf.EntityData;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused import

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Improvement Request for or addition/enhancement of a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants